Skip to content

refactor: fold heap-CMS count_events into aggregation_sub_type - #719

Open
milindsrivastava1997 wants to merge 4 commits into
mainfrom
dev-milind-2
Open

refactor: fold heap-CMS count_events into aggregation_sub_type#719
milindsrivastava1997 wants to merge 4 commits into
mainfrom
dev-milind-2

Conversation

@milindsrivastava1997

Copy link
Copy Markdown
Contributor

What

CountMinSketchWithHeap used to split its SUM-vs-COUNT weighting across two places: aggregation_sub_type (always the redundant literal "topk") and a separate parameters["count_events"] boolean. Plain CountMinSketch/MultipleSum already encode the same SUM/COUNT choice with aggregation_sub_type alone, so heap-CMS now does the same — aggregation_sub_type: "sum" | "count", no more count_events parameter.

Along the way:

  • Deleted AggregationType::SingleSubpopulation/MultipleSubpopulation — turned out to be dead code, only ever constructed in tests.
  • Added asap_types::aggregation_mode (AggregationMode/CountMode/MinMaxMode) as one typed place to interpret aggregation_sub_type, used by capability matching, the accumulator factory, and the planner instead of each re-parsing the string themselves.
  • Updated the planner (PromQL + SQL paths) and the cost-model/candidate-generation code to match.

No backward-compat shim needed — AggregationConfig is generated fresh by the planner per query, not persisted, so there's nothing old-format to keep reading.

Closes #670

Test plan

  • Full workspace cargo test green (asap_types, asap-planner-rs, asap-query-engine)
  • cargo clippy --workspace --all-targets clean
  • Planner integration tests exercise the real PromQL/SQL topk pipeline end-to-end, not just unit fixtures

🤖 Generated with Claude Code

milindsrivastava1997 and others added 4 commits September 4, 2026 22:22
…lation aggregation types (#670)

Both variants were only ever constructed in #[cfg(test)] code; the real
statistic-to-aggregation-type mapping never produces them. Removing them
also deletes their legacy factory arm that reused aggregation_sub_type as
an inner-accumulator-kind string and hardcoded SUM semantics for nested
CMS (issue #670, Finding 856), since the dead code is gone rather than
preserved.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K2SLcSk9UnZi5iAWKt26kH
…_type (#670)

CountMinSketchWithHeap previously split its SUM/COUNT weighting across two
places: aggregation_sub_type (fixed to the now-redundant "topk") and a
separate parameters["count_events"] boolean, duplicating and diverging from
how plain CountMinSketch/MultipleSum already encode the same axis via
sub_type alone. Hard-cutover to aggregation_sub_type: "sum"|"count" for all
three CMS-family types, removing count_events entirely.

Introduces asap_types::aggregation_mode (AggregationMode/CountMode/
MinMaxMode) as the single typed seam AggregationConfig::mode() exposes;
capability_matching's three separate weighting-compatibility functions and
accumulator_factory's duplicate sub_type parsers now route through it
instead of each re-deriving the same semantics.

The planner (promql.rs, sql.rs) patches a topk candidate's sub_type to the
detected weighting right after building it, since map_statistic_to_
precompute_operator's placeholder "topk" sub_type is only a stand-in until
the real weighting is known; candidate_gen.rs/atomic_costs.rs updated to
match. Backward-compat: none needed, AggregationConfig is regenerated by
the planner per query rather than persisted.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K2SLcSk9UnZi5iAWKt26kH
…b_type change

The Arroyo pipeline generator derives its UDF name as
f"{aggregationType}_{aggregationSubType}", and the topk-vs-non-topk column
selection patch keyed off aggregationSubType == "topk". Both broke once
CountMinSketchWithHeap's sub_type moved to "sum"/"count" (2e950f5, #670):
UDF lookup would 404 on the missing countminsketchwithheap_sum/_count
templates, and the column-selection patch would silently stop firing.

Renames the existing (COUNT-semantics) UDF template/function to
countminsketchwithheap_count, and switches the column-selection check to
aggregationType, which uniquely identifies a topk config regardless of its
subtype. A value-weighted (SUM) Arroyo UDF was never implemented -- the old
single template always used count semantics regardless of weighting -- so
countminsketchwithheap_sum intentionally has no template yet; that case now
fails loudly with a clear "template not found" error instead of silently
returning wrong (count-weighted) results, which is what actually happened
before this change.

Roborev: closes review 205.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Completes ae9c593 (the file rename alone accidentally landed as its own
commit): renames the UDF fn inside the template to match, switches the
topk column-selection patch from aggregationSubType == "topk" to
aggregationType == "countminsketchwithheap", and adds a regression test
covering both new sub_type values.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997 milindsrivastava1997 changed the title Fold heap-CMS count_events into aggregation_sub_type refactor: fold heap-CMS count_events into aggregation_sub_type Sep 5, 2026
@milindsrivastava1997
milindsrivastava1997 marked this pull request as ready for review September 5, 2026 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(asap-types): model aggregation kinds and subtypes with typed enums

1 participant